Skip to content

Conversation

@enguerranws
Copy link
Collaborator

@enguerranws enguerranws commented Dec 13, 2023

Todo:

  • add the story

@enguerranws enguerranws changed the title Fieldset handles RadioRichButtons Feature / RadioRichButtons Dec 13, 2023
@enguerranws enguerranws marked this pull request as draft December 13, 2023 16:43
@enguerranws enguerranws marked this pull request as ready for review December 18, 2023 08:48
options: {
label: ReactNode;
hintText?: ReactNode;
nativeInputProps: ComponentProps<"input">;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here it's best to use the ComponetProps type helper from React.

@garronej
Copy link
Collaborator

Hey @enguerranws,

Thanks a lot for the PR!

I first I did this review to improve consistency with the rest of the project: 765f233

But in the end I did even simpler.

I don't think we need to have a separate RadiRichButton. We can just infer it's rich by the use of illustations.
The drawback is that we can't enforce at the type level that if there is one radio that has an illustration all radio must have one but I think it's really no big deal.

@garronej garronej merged commit 13abe06 into main Dec 18, 2023
@garronej garronej deleted the feature/radio-rich-buttons branch December 18, 2023 19:04
@enguerranws
Copy link
Collaborator Author

I don't think we need to have a separate RadiRichButton. We can just infer it's rich by the use of illustations.
The drawback is that we can't enforce at the type level that if there is one radio that has an illustration all radio must have one but I think it's really no big deal.

I thought about that, but I wanted to stick the DSFR's components (where Radio buttons and Radio rich are presented as different components), but that's fine for me!

@garronej
Copy link
Collaborator

I thought about that, but I wanted to stick the DSFR's components (where Radio buttons and Radio rich are presented as different components), but that's fine for me!

Oh! Is that so? In that case you probably made the right call.
Anyway, now that it's published do you think I should rollback or keep it like this?

@garronej
Copy link
Collaborator

I can rollback the last commit if you think it's best. Idk

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants